Skip to content

Fix the failure to get trigger file if irrelevant window pops up - #1201

Merged
fbricon merged 4 commits into
redhat-developer:masterfrom
Vigilans:vigilans/fix-overview-breaks-invisible
Dec 18, 2019
Merged

fbricon merged 4 commits into
redhat-developer:masterfrom
Vigilans:vigilans/fix-overview-breaks-invisible

Conversation

@Vigilans

Copy link
Copy Markdown
Contributor

Fix #1198

Trigger file will now try to get documents from all opened text documents, if all trials from previous version failed.

Signed-off-by: Vigilans <vigilans@foxmail.com>
Comment thread src/extension.ts
for (const rootFolder of workspace.workspaceFolders) {
workspace.workspaceFolders.forEach(rootFolder => {
if (rootFolder.uri.scheme !== 'file') {
continue;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why? if subsequent folders have the file:// prefix, why should they be discarded?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmm so here return simply returns from the lambda, so it's fine

@Vigilans Vigilans Dec 18, 2019 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I change the for...of pattern to forEach, and here return is equivalent to continue, just to continue the outer loop...

@fbricon

fbricon commented Dec 18, 2019

Copy link
Copy Markdown
Collaborator

I can still see the bug when no java files have been opened and some commands triggered the activation of vscode-java.
As a last (ugly) resort, we might check for the existence of java files under a src/ folder at the root. Even then, that will not cover all cases, but we should cover more ground

@Vigilans

Copy link
Copy Markdown
Contributor Author

@fbricon Searching for java file needs getTriggerFiles to be async, do you think we need to move getTriggerFiles into resolveRequirements?

@fbricon

fbricon commented Dec 18, 2019

Copy link
Copy Markdown
Collaborator

it's not a requirement per se, but I'd be fine performing the lookup asynchronously in a different method

Signed-off-by: Vigilans <vigilans@foxmail.com>
@Vigilans

Copy link
Copy Markdown
Contributor Author

I've add java file searching in workspace as the last resort.

It searches all src folder in the workspace, whenever one java file is found, it will stop searching.

Comment thread src/extension.ts Outdated
}
});

for (const javaFile of await workspace.findFiles("**/src/**/*.java", undefined, 1)) { // Find at most 1 java file

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather be more conservative and look for src/**/*.java instead, or else if we might have some performance issues in case a very large folder has been opened

@Vigilans Vigilans Dec 18, 2019 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed to src/**/*.java now. Is trigger file only used for invisible project? If so, then indeed we can assume that src will cover most cases, since multi-module project will use a build tool instead.

But we may also search java files under the workspace root directory, since the user of invisible project may only create java files in the outermost place.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed to src/**/*.java now. Is trigger file only used for invisible project? If so, then indeed we can assume that src will cover most cases, since multi-module project will use a build tool instead.

yes src/**/*.java is only for invisible projects.

But we may also search java files under the workspace root directory, since the user of invisible project may only create java files in the outermost place.

yes makes sense

Signed-off-by: Vigilans <vigilans@foxmail.com>
Signed-off-by: Vigilans <vigilans@foxmail.com>
@Vigilans

Copy link
Copy Markdown
Contributor Author

Now trigger file of one specific workspace folder is being searched as following order:

  • Active text editor
  • Visible text editors
  • Workspace text documents
  • Java file in workspace root
  • Java file in src or test directory

and will early exit whenever one java file is found.

@fbricon

fbricon commented Dec 18, 2019

Copy link
Copy Markdown
Collaborator

what are workspace.textDocuments?

@Vigilans

Vigilans commented Dec 18, 2019 •

Copy link
Copy Markdown
Contributor Author

@fbricon Whenever workspace.openTextDocument(uri) is called, a text document is created in memory and added to workspace.textDocuments. It contains properties like languageId, isUntitled, isDirty and getText().

So using workspace.textDocuments can get documents opened but now shown in a TextEditor.

@fbricon

fbricon commented Dec 18, 2019

Copy link
Copy Markdown
Collaborator

in which circumstance could we have a Java file present in in textDocuments but not in the list of visible text editors, that would need to trigger the language server?

@Vigilans

Copy link
Copy Markdown
Contributor Author

It is the first situation that this PR tries to resolve: #1198

When opening a java file to activate the extension, the Java Overview pops up, and the java file goes to background.

In this circumstance:

  • window.activeTextEditor is undefined, because Java Overview is a webview, not a text editor.
  • window.visibleTextEditors does not contain the java file, because the java file is hidden by Java Overview.
  • workspace.textDocuments does contain the java file, because this file has been opened, althogh it is not in a visible state.

@fbricon
fbricon merged commit fd3de87 into redhat-developer:master Dec 18, 2019
@fbricon

fbricon commented Dec 18, 2019

Copy link
Copy Markdown
Collaborator

Thanks @Vigilans!

@Vigilans
Vigilans deleted the vigilans/fix-overview-breaks-invisible branch December 19, 2019 01:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Java Overview will break the import of invisible project

2 participants